Repository navigation
docs: language-agnostic PROTOCOL.md wire spec - #578
Conversation
Address all five review findings: did:key-only resolution (did:web / did:gitlawb marked planned), SHA-1 object format with IPNS demoted to planned and the REST refs CID mapping described instead, the receive-pack route and the 404-fetch vs 401-push auth split documented, iCaptcha wire shape corrected (client sends only x-icaptcha-proof; url/level are 403 response headers; ICAPTCHA_MODE defaults off over a small endpoint set), and the core gitlawb/ref-update/v1 schema spelled out field-for-field with node push receipts explicitly split off as a different document.
- @path binds path plus query, not RFC 9421's path-only value; a second implementation signing info/refs without ?service= fails verification - content-digest is a mandatory covered component; a body-less request signs the SHA-256 of the empty body, also accepted when absent - TLS transport stated in the signature section with a pointer from smart-HTTP - the ref-update certificate section is marked planned: the schema is defined in code but no shipped endpoint serves it, and its 64-hex hash fields reserve the SHA-256 width while repos are sha1 today - smart-HTTP documents the mandatory ?service= values, pkt-line framing, and the upload-pack/receive-pack result content types
|
Warning Review limit reachedThis review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Next included review available in 46 minutes. View limit details
|
|
@coderabbitai full review |
Action not completed
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- GitHub reports merge state BLOCKED (branch protection / required reviews). No human review threads on the head yet; CodeRabbit hit rate limits and did not review the diff.
- Rust CI matrix skipped (
fmt + clippy,test,build, etc.) becausechanged surfacesdid not flag Rust paths — expected for a docs-only diff, but note that this review’s implementation cross-check is static, not a freshcargo testrun on head. - Branch head merges current
mainat base4fc041a(same as merge-base captured here). No three-dot conflict signal; no duplicate open sibling besides this takeover of #372. - Linked motivation #367 (wire spec for second implementations) is satisfied only if PROTOCOL stays aligned with the cited crates; findings below are accuracy gaps on that bar.
Scope and attribution (not pre-existing main work)
All findings below are documentation defects in new or changed PROTOCOL.md copy. They are not requests to change production behavior on main.
| Finding | Shipped behavior on main (unchanged) |
What this PR broke or omitted | Author may edit |
|---|---|---|---|
| Content-Digest | Header required on every signed request (verify_signature + unit test) |
1a576ea §2 claims body-less requests may omit the header |
PROTOCOL.md (+ PR body bullets if they repeat the claim) |
| Path normalization | .git strip + owner shortening in smart_http_repo_name / get_repo |
§5 templates omit rules needed for correct @path signing |
PROTOCOL.md |
| TLS | No TLS/scheme gate in auth (signatures verified regardless of transport) | 1a576ea §2/§5 read like the node rejects cleartext signatures |
PROTOCOL.md |
| iCaptcha inert enforce | Fail-safe: enforce with no pubkey stays inert (icaptcha.rs) |
§3 omits that branch; over-broad “when proof is required” | PROTOCOL.md |
Explicitly not in scope for this PR (would be drift or pre-existing main debt): changing verify_signature, adding TLS middleware, altering path normalization or iCaptcha fail-safe policy, documenting full HTTP status taxonomy (400 vs 401 splits), post-signature 403 owner/UCAN push rules, X-Ucan delegation, 503/429 shedding, or gossip/async CID semantics. Those surfaces were searched and left out of Findings on purpose.
Findings
🟠 P1 — Signed requests must send Content-Digest; empty body does not excuse a missing header
📍 Where: PROTOCOL.md §2 Integrity (lines 31–35); verifier crates/gitlawb-node/src/auth/mod.rs (verify_signature); regression test in the same module.
💥 What fails: A second implementation reading §2 can sign the empty-body digest in Signature-Input but omit the Content-Digest header (as the doc allows). The node answers 400 with missing_content_digest and never reaches handlers — including git-receive-pack and signed REST writes.
let content_digest = match parts
.headers
.get("content-digest")
.and_then(|v| v.to_str().ok())
{
Some(v) => v.to_string(),
None => {
return (
StatusCode::BAD_REQUEST,
Json(json!({
"error": "missing_content_digest",
"message": "Content-Digest header is required on a signed request",
})),
)
.into_response()
}
};
🔎 Root cause: §2 mixes “content-digest is a mandatory covered component” (correct) with “the node also accepts when the header is absent entirely” on body-less requests (false). The middleware always binds content-digest in the signing string from the header value, not from an inferred empty-body digest alone.
📜 Stated contract:
PR #578: land the wire spec so it matches the implementation it describes; maintainer commit explicitly documents mandatory
content-digestcoverage and the empty-body digest value.
auth/mod.rsunit test: a signed request with noContent-Digestheader must be refused (missing_content_digest).
🏷️ Attribution: PR-introduced inaccuracy — sentence added on head 1a576ea. Pre-1a576ea branch text only said digest is “verified when the header is present” and did not authorize omission. Middleware behavior is unchanged pre-existing on main; only the new doc text is wrong.
📌 In this PR:
PROTOCOL.md§2 Integrity bullet on body-less requests and absentContent-Digest- PR description / commit message if they still say the header may be absent on empty bodies
🔒 Unchanged on main: verify_signature and its regression test — no Rust changes in this docs PR.
🔧 Required correction: Rewrite §2 so conforming clients always send Content-Digest: sha-256=:…: on signed requests. For empty bodies, state that the value is the SHA-256 digest of the empty body and that the header is still required. Remove any wording that permits omitting the header while still signing content-digest.
🛠️ Author fix: Close the root cause on every §2 sentence that touches mandatory components or body-less signing in one pass. Align the PR description bullet list with corrected §2 so the absent-header claim does not return in prose outside the file.
🚫 Out of scope: Changing whether the node accepts header-less empty-body signatures; edits to gitlawb-core signing helpers.
🟡 P2 — Smart-HTTP URL shape must document .git suffix stripping and owner DID shortening
📍 Where: PROTOCOL.md §5 Smart-HTTP endpoints; crates/gitlawb-node/src/api/repos.rs (smart_http_repo_name); crates/gitlawb-node/src/db/mod.rs (get_repo).
💥 What fails: Git clients and git-remote-gitlawb commonly use /{owner}/{repo}.git/info/refs?service=… and the bare multibase segment of the owner did:key in the path. §5 shows only /{owner}/{repo}/… with no normalization rules. A signer that copies the doc literally may use a full did:key:… owner or the wrong repo spelling, producing @path mismatches and signature verification failures even when the HTTP request is otherwise valid.
🔎 Root cause: The spec documents route templates but not the at-most-once .git strip or owner-key shortening the node already applies — omission in new docs, not missing implementation.
📜 Stated contract:
crates/gitlawb-core/src/http_sig.rs:@pathis the request path and query used in verification.
ucan::push::canonical_repo_name(viasmart_http_repo_name): smart-HTTP and UCAN push capabilities share the same.gitnormalization, including thefoo.git→foo.git.gitedge case.
🏷️ Attribution: PR-introduced omission in new PROTOCOL.md §5. Lookup/normalization code is unchanged pre-existing on main.
📌 In this PR:
PROTOCOL.md§5 Smart-HTTP endpoint paths (allGET/POSTtemplates)PROTOCOL.md§2@pathexample (replace placeholder/r/…with a signable/{owner}/{repo}/…example once normalization is documented)
🔒 Unchanged on main: smart_http_repo_name, get_repo, UCAN matchers.
🔧 Required correction: Add a Path normalization subsection under §5 (or §1) stating: (1) {repo} may include one trailing .git suffix, stripped before lookup; (2) {owner} may be full did:key:… or the bare multibase segment accepted by get_repo / did_matches; (3) @path must equal the exact path_and_query bytes on the wire, including ?service=. Include one worked example (short owner + .git + ?service=git-upload-pack).
🛠️ Author fix: Update §5 paths and the §2 @path example together so every listed URL is signable as written.
🚫 Out of scope: Changing normalization code; cataloguing every REST route.
🟡 P2 — TLS transport language overstates what the node enforces on the wire
📍 Where: PROTOCOL.md §2 Transport; §5 cross-reference to §2.
💥 What fails: §2 reads as if the node refuses HTTP signatures on cleartext. verify_signature has no TLS or scheme check; git-remote-gitlawb tests verify signatures over http:// request targets. Operators may terminate TLS in front of the service; implementers must not assume the Rust binary rejects cleartext at the signature layer.
🔎 Root cause: 1a576ea added transport bullets to the normative spec without separating deployment expectation from application behavior. Wording problem in new doc — not a defect to fix by adding TLS enforcement code.
📜 Stated contract:
PR #578: spec must match shipped implementation.
§2 as written: "the node does not terminate HTTP signatures over cleartext" (reads as node-level rejection).
🏷️ Attribution: PR-introduced wording in 1a576ea. No TLS gate on main before or after; not a pre-existing code bug filing.
📌 In this PR:
PROTOCOL.md§2 Transport bulletPROTOCOL.md§5 sentence that points signed smart-HTTP traffic at §2 TLS
🔒 Unchanged on main: auth stack; no new middleware.
🔧 Required correction: State that production deployments should use TLS, while the reference node verifies RFC 9421 headers on whatever transport reaches Axum (typically HTTPS via a reverse proxy). Do not imply the service itself refuses cleartext signatures.
🛠️ Author fix: Edit §2 and the §5 cross-reference in the same pass.
🚫 Out of scope: TLS termination, HSTS, or scheme checks in the node binary.
🟡 P2 — iCaptcha enforce without a loaded verifier key stays inert (undocumented fail-safe)
📍 Where: PROTOCOL.md §3; crates/gitlawb-node/src/icaptcha.rs (init, decide).
💥 What fails: §3 implies 403 icaptcha_proof_required whenever proof is missing under an active gate. With ICAPTCHA_MODE=enforce and no loaded pubkey (ICAPTCHA_PUBKEY / /v1/pubkey both fail), decide allows the request (inert gate, startup warning). Operators and second implementations can misread §3 as “enforce always 403s without proof.”
🔎 Root cause: §3 documents enforce happy-path only; it omits the documented fail-safe (“stay inert rather than rejecting every request”). Shadow mode intentionally never 403s — not a spec bug if §3 names enforce + loaded key as the condition for 403.
📜 Stated contract:
icaptcha.rs: if no public key is loaded, stay inert rather than rejecting every request.
🏷️ Attribution: PR-introduced omission in new §3. Fail-safe policy is unchanged pre-existing on main.
📌 In this PR:
PROTOCOL.md§3 Mode / Wire shape (qualify when403applies; mention inert enforce)
🔒 Unchanged on main: fail-safe in icaptcha.rs.
🔧 Required correction: Document that 403 icaptcha_proof_required applies in enforce mode only when a verifier key is loaded. Document inert enforce (admission same as off; operator warnings at startup). Tie “proof required” language to enforce + key, not shadow.
🛠️ Author fix: Amend §3 in one edit; do not imply shadow returns 403.
🚫 Out of scope: Requiring a key before enforce or changing fail-safe policy.
- Content-Digest is required on every signed request: the node refuses a signed request without it (400 missing_content_digest); a body-less request still sends the SHA-256 of the empty body, never omits it - transport: deployments should run TLS in front, but the node verifies signatures on whatever transport reaches it, no scheme gate - path normalization documented: one trailing .git stripped, owner may be full did:key or bare multibase, and @path is the literal path_and_query bytes; the signing example is now signable as written - icaptcha enforce with no loaded verifier key stays inert, so the 403 reads as enforce-plus-loaded-key, not enforce alone
|
Addressed on Content-Digest: the "accepted when absent" wording was wrong on my edit — Transport: §2 now splits the deployment expectation (production should run TLS in front, typically a reverse proxy) from application behavior: Path normalization: §5 has a new subsection covering the at-most-once iCaptcha: §3 now conditions the |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Rechecked head 1213ef8658e215a14e309e0bd1099c9b3bb84bcd. The prior change-request items on mandatory Content-Digest, transport semantics in §2, smart-HTTP path normalization, and iCaptcha fail-safe match the implementation on this head and are not re-opened below. One in-diff operator-copy mismatch remains.
Merge readiness
- GitHub merge state BLOCKED (branch protection / required reviews). Prior
CHANGES_REQUESTEDreview was on1a576ea; re-approval needed on current head. - Rust CI matrix skipped (
fmt + clippy,test,build, etc.) becausechanged surfacesdid not flag Rust paths — expected for a docs-only diff. This review’s cross-check is static against the cited crates, not a freshcargo testrun on head. - Branch head is current with live
mainat merge-base4fc041a(mergeable, no three-dot conflict). Wire-spec intent is carried on this branch (successor to #372). - CodeRabbit rate limited on the head run; no automated diff review to reconcile.
Scope and attribution (not pre-existing main work)
All Findings below are documentation defects in new or changed PROTOCOL.md copy. They are not requests to change production behavior on main.
| Topic | Shipped behavior on main (unchanged) |
Status on head 1213ef86 |
|---|---|---|
| Content-Digest | Header required on every signed request (verify_signature + unit test) |
Resolved — §2 matches |
| Path normalization | .git strip + owner shortening in smart_http_repo_name / get_repo |
Resolved — §2 example + §5 |
| Transport / scheme | No TLS/scheme gate in auth | Resolved in §2 — §5 cross-ref still wrong → Finding below |
| iCaptcha inert enforce | Fail-safe: enforce with no pubkey stays inert (icaptcha.rs) |
Resolved — §3 |
Explicitly not in scope for this PR (would be drift or pre-existing main debt, not actionable here):
- Changing
verify_signature, adding TLS/scheme middleware, or altering path normalization / iCaptcha policy in Rust - Full HTTP status taxonomy (
400vs401vs404after visibility), post-signature 403 owner / UCAN push rules,X-Ucan, 503/429 shedding, shadow-mode wire semantics beyond what §3 states - Gossip, IPNS on-wire semantics, exhaustive auth error catalogs (clock skew,
not_an_agentshape, etc.) - Visibility 404 after a valid signature on push — unchanged pre-existing behavior; §5’s “401 for missing/invalid signatures” on push POST is not a claim that all push-related routes return 401
Those surfaces were searched; withholding them is not “missing findings,” it is keeping this PR surgical.
Findings
🔵 P3 — §5 must not re-require TLS on signed smart-HTTP after §2 clarifies there is no scheme gate
📍 Where: PROTOCOL.md §5 Smart-HTTP (POST …/git-receive-pack bullet, line 111); sibling §2 Transport (lines 19–22).
💥 What fails: §5 still ends with “Signed requests on this path follow §2, including TLS transport.” A reader treating §5 as normative can infer TLS is part of the signed push contract even after §2 correctly states the verifier has no scheme check and does not refuse cleartext at the signature layer. That contradicts the same document and encourages second implementations to reject interop setups the node accepts (for example signed requests over http:// in tests).
🔎 Root cause: The maintainer fix on 1213ef86 corrected §2 transport language but left the §5 cross-reference wording from 1a576ea, so operator-facing transport rules disagree within one file.
📜 Stated contract:
Issue #367 / PR #578: land a wire spec that matches the implementation it describes.
§2 on head: “The node itself has no scheme check … nothing at the signature layer refuses a cleartext request.”
🏷️ Attribution: Incomplete claimed fix — 1213ef86 updated §2 but not the §5 sibling. Verifier behavior is unchanged pre-existing on main; only in-diff copy is wrong.
📌 In this PR:
PROTOCOL.md§5 receive-pack bullet cross-reference to §2 (line 111)
🔒 Unchanged on main: verify_signature and smart-HTTP routing — no Rust changes in this PR.
🔧 Required correction: Point signed git-receive-pack traffic at §2 for RFC 9421 header rules only. Remove normative “including TLS transport” (or replace with non-normative deployment guidance that defers to §2 and does not imply the verifier enforces HTTPS).
🛠️ Author fix: Edit the §5 bullet and any neighboring sentence that still ties signed smart-HTTP to mandatory TLS in one pass. Keep §2 transport wording as-is.
🚫 Out of scope: TLS termination in the node, HSTS, scheme middleware, or expanding §2/§5 into a full HTTP status taxonomy.
§5 still told readers signed git-receive-pack traffic follows §2 "including TLS transport" after §2 was corrected to state the verifier has no scheme check. A second implementation could infer TLS is part of the signed push contract and reject cleartext requests the node accepts. Point the path at §2's RFC 9421 header rules and defer transport policy to §2's non-normative deployment note.
|
Addressed on 61b9c4b. The receive-pack bullet now points at the RFC 9421 header rules in section 2 only and defers transport policy to section 2's deployment note, so the file no longer tells a second implementation that the signature layer requires TLS. grep -n TLS PROTOCOL.md shows only the non-normative guidance remains. |
|
@coderabbitai full review |
Action not completed
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
- GitHub reports merge state BLOCKED (branch protection / required reviews). Re-request review after merge; prior
CHANGES_REQUESTEDfrom jatmn targeted1213ef86and is superseded by head61b9c4b27c5ba08e9063c7c58e21b89602c83662(including the §5/§2 TLS cross-reference fix). - Rust CI matrix skipped (
fmt + clippy,test,build, etc.) becausechanged surfacesdid not flag Rust paths — expected for a docs-only diff. This review’s implementation cross-check is static against the cited crates, not a freshcargo testrun on head. - Branch is mergeable with live
origin/mainat72c31ddeca87cfa85ad523772de5c6728166fd98; merge-base with PR base is4fc041a3995965e199c5dc49d276717b40ab111a. Successor to #372 for #367 wire-spec intent. - CodeRabbit rate-limited on the head run; no automated diff review to reconcile.
Scope note
PROTOCOL.md intentionally documents the shipped alpha slice (identity, RFC 9421 rules used on push/register/create, iCaptcha on three write entrypoints, planned ref-update schema vocabulary, smart-HTTP). It does not attempt the full #367 outline (complete REST catalog, UCAN delegation, P2P, exhaustive status/error taxonomy). Gaps called out in blind search (optional-signature fetch, post-signature 403 owner/UCAN push rules, shadow-mode observability, full REST refs path) are unchanged main behavior or informational deferrals — not accuracy defects in the claims this PR makes.
Verification performed
Cross-checked §1–§5 against crates/gitlawb-node/src/auth/mod.rs, crates/gitlawb-core/src/http_sig.rs, crates/gitlawb-node/src/icaptcha.rs, crates/gitlawb-core/src/cert.rs, crates/gitlawb-node/src/api/repos.rs (smart-HTTP), crates/gitlawb-node/src/git/store.rs, and crates/gitlawb-node/src/server.rs routing. Prior review items on mandatory Content-Digest, @path+query, path normalization, §2 transport vs verifier behavior, iCaptcha inert enforce, and §5 TLS wording are aligned on this head.
Summary
Adds
PROTOCOL.md: a language-agnostic wire specification for what the nodeships today (alpha), carried over from #372 with the outstanding review
findings applied so the spec matches the implementation it describes.
Motivation & context
A second implementation cannot interop without a spec of the shipped wire
behavior. The original PR documents identity, RFC 9421 authentication, the
iCaptcha gate, the ref-update certificate schema, and smart-HTTP. Review
rounds verified most of it against the code; this branch lands the remaining
accuracy fixes. Addresses #367.
Kind of change
What changed
PROTOCOL.md(new, contributor file): identity anddid:keyscope, RFC9421 signature coverage, iCaptcha wire shape, ref-update certificate
schema, smart-HTTP endpoints, storage notes.
finding:
@pathdocumented as path plus query string, matching the node'spath_and_query()binding rather than RFC 9421's path-only value.content-digestmarked mandatory across the covered-component set, withthe empty-body digest value spelled out for body-less requests.
smart-HTTP section.
defined in code but no shipped endpoint serves it, and its 64-hex hash
fields reserve the SHA-256 object width while served repositories are
sha1 today.
?service=parameter and its twoaccepted values, pkt-line framing, and the
application/x-git-upload-pack-result/application/x-git-receive-pack-resultcontent types.How a reviewer can verify
Docs-only; no code changed on the maintainer commits.
PROTOCOL.mdsection 2 againstcrates/gitlawb-node/src/auth/mod.rs(
path_and_querybinding, mandatory covered components) andcrates/gitlawb-core/src/http_sig.rs.crates/gitlawb-node/src/icaptcha.rsand the gated callsites.
crates/gitlawb-core/src/cert.rs(field names, 64-hexrequirement, signature list).
crates/gitlawb-node/src/api/repos.rs(?service=handling, pkt-line, result content types) and
crates/gitlawb-node/src/git/store.rs(
--object-format=sha1).Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNone of the protocol surface changes: this is documentation of existing wire
behavior only.
Notes for reviewers
Docs-only PR. The cert section is intentionally marked planned because
gitlawb-attestdefines the schema but no shipped endpoint serves thosecertificates today.